Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EMSCRIPTEN_EXPORT option #4977

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add EMSCRIPTEN_EXPORT option #4977

wants to merge 1 commit into from

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 25, 2017

From the mailing list discussion, EMSCRIPTEN_KEEPALIVE isn't the best name, so adding another alias makes sense. Also use this in the docs.

@dschuff
Copy link
Member

dschuff commented Feb 25, 2017

can we eventually get rid of EMSCRIPTEN_KEEPALIVE?

@dschuff
Copy link
Member

dschuff commented Feb 25, 2017

oh, this is attribute(used)). Hm I guess keepalive really is the best name for what the attribute actually does. Then the weird thing is that this also means export. So yeah it's even more important to have EMSCRIPTEN_EXPORT so that if we want to change the mechanism by which we export (e.g. to minimize the number of exports that aren't needed) we can.

@floooh
Copy link
Collaborator

floooh commented Feb 25, 2017

Just my 2ct: I think the name EMSCRIPTEN_EXPORT is useful because it gives an association to the EXPORTED_FUNCTIONS linker-stage command line option, and it also is similar to DLL_EXPORT (from the Windows world).

@kripken
Copy link
Member Author

kripken commented Feb 27, 2017

I think we never needed to differentiate exporting from keeping alive, doing both always made sense in practice. If we do want to split them up as @dschuff suggests, we probably shouldn't merge this as it would mean we need to change what this macro does later.

@floooh
Copy link
Collaborator

floooh commented Feb 27, 2017

I'm fine with not merging this yet if EMSCRIPTEN_EXPORT would need to change later anyway. I have my code annotated now with EMSCRIPTEN_KEEPALIVE and it's working fine.

@sbc100
Copy link
Collaborator

sbc100 commented Aug 19, 2019

Are we still interested in landing this?

@kripken
Copy link
Member Author

kripken commented Aug 19, 2019

I'm not sure.

Are we adding clang intrinsics for wasm imports and exports? Those would probably be better than this if so.

@dschuff
Copy link
Member

dschuff commented Aug 19, 2019

does attribute(used)) still have any special meaning other than "don't DCE this function"? i.e. does it force thing to be exported? i know we've changed a lot of the logic around exporting things (e.g. especially differentiating things exported from a library vs the compiled executable).

@sbc100
Copy link
Collaborator

sbc100 commented Aug 19, 2019

Yes, currently attribute(used)) means force wasm export. We probably want to have a separate attribute which is wasm-specific.

It seems like whatever the final mechanism we decide to under the hood we still want an emscripten-specific macro like this for some added indirection and explicitness.

@sbc100 sbc100 closed this Jan 30, 2020
@sbc100
Copy link
Collaborator

sbc100 commented Jan 30, 2020

The deletion of the incoming branch triggered the automatic closing of this branch. My apologies. This was an unexpected side effect.

Re-opening and re-targeting to master.

@sbc100 sbc100 reopened this Jan 30, 2020
@sbc100 sbc100 changed the base branch from incoming to master January 30, 2020 20:27
@sbc100
Copy link
Collaborator

sbc100 commented Jan 30, 2020

I think we should still land this. Can you rebase?

@kripken
Copy link
Member Author

kripken commented Jan 30, 2020

I think maybe we want to do this as part of a larger exporting refactoring, #8380 so best to wait on that (for which I'm not sure when I'll have time).

@stale
Copy link

stale bot commented Jan 30, 2021

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Jan 30, 2021
@dschuff
Copy link
Member

dschuff commented Feb 2, 2021

Are we still using __attribute__((used))? if so we should probably keep this open?

@stale stale bot removed the wontfix label Feb 2, 2021
@kripken
Copy link
Member Author

kripken commented Feb 2, 2021

Yes, it would be good to get around to this. But I think #8380 is the better idea, to do a larger refactoring of export names. Unless there is incremental benefit to this PR, I think we can close it.

@sbc100
Copy link
Collaborator

sbc100 commented Feb 2, 2021

I think having a better name than EMSCRIPTEN_KEEPALIVE to use in the source code is interdependently a good change. #8380 seems to deal only with external/link-time settings, rather than source annotations.

Base automatically changed from master to main March 8, 2021 23:49
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Apr 16, 2022
@sbc100
Copy link
Collaborator

sbc100 commented Apr 17, 2022

I still think we should do this

@stale stale bot removed the wontfix label Apr 17, 2022
@Thaina
Copy link

Thaina commented Apr 13, 2023

Is EMSCRIPTEN_KEEPALIVE actually equivalence to EMSCRIPTEN_EXPORT and -sEXPORTED_FUNCTIONS ?

I am using 2.0.19 in unity and EMSCRIPTEN_KEEPALIVE annotation seem to not export PoC function in my .c file. I need to use -sEXPORTED_FUNCTIONS args. And so I feel uncomfortable to have no explicit EMSCRIPTEN_EXPORT annotation that really export the function I want

@sbc100
Copy link
Collaborator

sbc100 commented Apr 13, 2023

Is EMSCRIPTEN_KEEPALIVE actually equivalence to EMSCRIPTEN_EXPORT and -sEXPORTED_FUNCTIONS ?

I am using 2.0.19 in unity and EMSCRIPTEN_KEEPALIVE annotation seem to not export PoC function in my .c file. I need to use -sEXPORTED_FUNCTIONS args. And so I feel uncomfortable to have no explicit EMSCRIPTEN_EXPORT annotation that really export the function I want

Yes EMSCRIPTEN_KEEPALIVE works just like -sEXPORTED_FUNCTIONS. Note, though, that if you code lives in a library then the object file needs to be referenced or the linker will ignore it. Is your object file part of a .a library? If so, you maybe need to -Wl,--whole-achive -lfoo -Wl,--no-whole-archive.

@Thaina
Copy link

Thaina commented Apr 13, 2023

Thank you for your clarification. But then I could say it then not equivalence. Because with -sEXPORTED_FUNCTIONS arguments I don't need to use anymore special complicate flags even that function was not used anywhere else

To be honest I try to use EMSCRIPTEN_KEEPALIVE because I don't want to add any flags into emscripten compile process. As I am using emscripten inside unity and modifying compiler flags is somewhat inconvenient. So if using EMSCRIPTEN_KEEPALIVE still require compiler flags it then defeat the purpose

I hope there could be some annotation that can explicitly force function to being export and not being trim out of optimization process. And I wish EMSCRIPTEN_EXPORT would do that

@sbc100
Copy link
Collaborator

sbc100 commented Apr 13, 2023

Thank you for your clarification. But then I could say it then not equivalence. Because with -sEXPORTED_FUNCTIONS arguments I don't need to use anymore special complicate flags even that function was not used anywhere else

To be honest I try to use EMSCRIPTEN_KEEPALIVE because I don't want to add any flags into emscripten compile process. As I am using emscripten inside unity and modifying compiler flags is somewhat inconvenient. So if using EMSCRIPTEN_KEEPALIVE still require compiler flags it then defeat the purpose

I hope there could be some annotation that can explicitly force function to being export and not being trim out of optimization process. And I wish EMSCRIPTEN_EXPORT would do that

Is the object file in question part of a .a achive?

@Thaina
Copy link

Thaina commented Apr 13, 2023

Thank you for your clarification. But then I could say it then not equivalence. Because with -sEXPORTED_FUNCTIONS arguments I don't need to use anymore special complicate flags even that function was not used anywhere else
To be honest I try to use EMSCRIPTEN_KEEPALIVE because I don't want to add any flags into emscripten compile process. As I am using emscripten inside unity and modifying compiler flags is somewhat inconvenient. So if using EMSCRIPTEN_KEEPALIVE still require compiler flags it then defeat the purpose
I hope there could be some annotation that can explicitly force function to being export and not being trim out of optimization process. And I wish EMSCRIPTEN_EXPORT would do that

Is the object file in question part of a .a achive?

I am not so sure and I guess it's not. There is no obvious .a archive generated from unity that I can see. The only result from unity is .wasm .loader.js .framework.js and .data

And what I know is only that append -sEXPORTED_FUNCTIONS to PlayerSettings.WebGL.emscriptenArgs make it work. While putting EMSCRIPTEN_KEEPALIVE on the C function did not work

@sbc100
Copy link
Collaborator

sbc100 commented Apr 13, 2023

And what I know is only that append -sEXPORTED_FUNCTIONS to PlayerSettings.WebGL.emscriptenArgs make it work. While putting EMSCRIPTEN_KEEPALIVE on the C function did not work

Well this I think we can assume that unity is building .a archive under the hood since that is the only way that EMSCRIPTEN_KEEPALIVE wouldn't have the same effect as -sEXPORTED_FUNCTIONS. @juj would know for sure.

Sadly, for symbols that live inside of archive files, when the linker skips the entire object file all the information in it is completely skipped, including any symbol attributes. Making EMSCRIPTEN_KEEPALIVE for symbol in archive files which are skipped would be highly non trivial. The solutions we have today are:

  1. Make sure the object file in question is references by some other symbol which is included (this triggers the object file to be included)
  2. Force the include of all objects using --whole-archive
  3. Force the include of the symbol using -ufoo (where foo is the name of the sybmol).

Sadly both (2) and (3 ) require custom command line flags. (1) might be an option is you have at least one object file that you know will be included than you can include dummy references to symbols in question. For example:

void* dummy = &myfunc;

Include that line in a file that you know will be included at link time will cause the linker to include the object file that contains myfunc.

@Thaina
Copy link

Thaina commented Apr 13, 2023

Thank you for your suggestion. Still I wish that there could be some non hacky way for this. IMHO I think the function I expected to be export should be defined at the function declaration itself, not the compiler flags, regardless of archive generation method

I would try to see which way is suitable for developing in unity webgl. Thanks for all enlightened information

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we land this ancient PR? I think it probably useful regardless of what we do with command line flags.

#include <emscripten.h>
#include <math.h>

extern "C" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this into a .c file avoid this extern "C" block.

@dschuff
Copy link
Member

dschuff commented Jan 18, 2024

Yeah, I think this is worth having (and worth naming correctly) if we're confident we can support it going forward (and I think we can, even if LLVM manages to replace __attribute__(used) I'm sure there would be some replacement).
We should have some documentation on how and where it can be used; e.g. only functions? (does it need go on declaration, definition, or both?), how to use with data, tables? can it go on classes, etc

@kripken
Copy link
Member Author

kripken commented Jan 18, 2024

If we are certain we can replace attribute(used) then adding this alias for EMSCRIPTEN_KEEPALIVE seems fine. If we aren't certain, though, then I still think it is safer to first see what happens with the attributes, as that would give us the option for a new EMSCRIPTEN_EXPORT to use a new attribute, avoiding a breaking change.

@dschuff
Copy link
Member

dschuff commented Jan 26, 2024

I think the idea would be not that we are sure that we can replace attribute(used) (because IIUC the current problem is that other attribute-based options only apply to functions and not to global data), but that we want to replace the EMSCRIPTEN_KEEPALIVE name, and if EMSCRIPTEN_EXPORT only ends up ever expanding to attribute(used) as EMSCRIPTEN_KEEPALIVE does today, then that would be ok.

Are you saying that in the future we might want to switch to a different mechanism other than attribute(used), which might end up being a breaking change, so we should avoid introducing new user-facing names until that time?

@kripken
Copy link
Member Author

kripken commented Jun 10, 2024

@dschuff

Are you saying that in the future we might want to switch to a different mechanism other than attribute(used), which might end up being a breaking change, so we should avoid introducing new user-facing names until that time?

Sorry, I thought I'd replied to this long-ago notification, but it seems I did not. Yes, that was what I was saying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants